Skip to content

Conversation

abhishektrip
Copy link
Contributor

@abhishektrip abhishektrip commented Apr 3, 2018

Related issue

Description of changes

Fixes following issues

  • No indication of nesting in AbstractMap
  • AbstractMap foldouts close in play mode
  • Sample count and Relative Height under terrain options need tooltips
  • New Visualizers Keep Settings from Previous Ones
  • Empty Visualizer Name is Possible

QA checklists

  • Add relevant code comments. Every API class and method should have <summary> description as well as description of parameters.
  • Add tests for new/changed/updated classes and methods!!!
  • Check out conventions in CONTRIBUTING.md.
  • Check out conventions in CODING-STYLE.md
  • Update the changelog
  • Update documentation.

Reviewers

Tag your reviewer(s). Choose wisely.

* commit '712cd80dd287588b40b164b20ab4ced121d79ffc':
  Fix range height extrusion (#613)

# Conflicts:
#	sdkproject/Assets/Mapbox/Unity/DataContainers/MapboxEnums.cs
[Serializable]
public class ElevationModificationOptions
{
[Tooltip("Number of samples to use for terrain mesh.")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it mean to use X number of samples? Can we add more context here?

Copy link

@jim-martin jim-martin Apr 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be better described as 'resolution' or 'mesh resolution'. The mesh is generated by sampling the tile elevation image 'x' number of times at even intervals.

So '2' would mean a 2x2 grid of vertices per tile, and '10' would mean a 10x10 grid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So " Mesh resolution of terrain, results in n x n grid " . I am so bad at defining this ;)

[Description("Extrude features using the property value.")]
PropertyHeight,
[Description("Extrude features using the property value. Values lower than min value are clamped at min value.")]
[Description("Extrude features using the property value. Extrusion from features minimum height value. Results in flat tops.")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Extrusion from features minimum height value" -> can we phrase this differently? Is it fair to say "Sets the minimum height of extrusions"? Same comment for max height ("Sets the maximum...")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this doesn't set extrusions minimum value. Choses the vertex with minimum height and extrudes the height from there for all features.
For example, if you had a building on a hill, the footprint will have different heights at different corners, when choosing MinHeight extrusion, the lowest vertex will be extruded by the "height" value and the rest will be extruded to a value which results in flat tops.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh got it. Now I better understand your definition. How about:

"Sets height based on property's minimum height, if height isn't uniform"
"Sets height based on feature's minimum height"
"Choses property's minimum height and sets as uniform height"
"Choses property's minimum height and sets that as base for extruding all features"

Pick whatever you think works best!

subLayerMaterialOptions.FindPropertyRelative("materials").ClearArray();
subLayerMaterialOptions.FindPropertyRelative("materials").arraySize = 2;
subLayerMaterialOptions.FindPropertyRelative("atlasInfo").objectReferenceValue = null;
subLayerMaterialOptions.FindPropertyRelative("colorPallete").objectReferenceValue = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

colorPalette* (not colorPallete)

Copy link
Contributor

@morgane morgane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly come copyediting tweaks

@jim-martin
Copy link

  1. No indication of nesting in AbstractMap
    • Can we also include indents in the Others foldouts in GENERAL and TERRAIN?
  2. AbstractMap foldouts close in play mode
    • The foldouts remain open, but users must still re-select their layer from from the visualizer list. Can we retain the layer selection as well?
  3. Sample count and Relative Height under terrain options need tooltips.
  4. New Visualizers Keep Settings from Previous Ones
    • Fixed. Bonus - automatically select the newly created visualizer.
  5. Empty Visualizer Name is Possible
    • Fixed 👍

Copy link

@jim-martin jim-martin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested in earlier comment.

@abhishektrip
Copy link
Contributor Author

@jim-martin @morgane added some comments to your review. Please review them when you get a chance.

No indication of nesting in AbstractMap
Can we also include indents in the Others foldouts in GENERAL and TERRAIN?

IMO they are settings at the same level, they should not be tabbed.

AbstractMap foldouts close in play mode
The foldouts remain open, but users must still re-select their layer from from the visualizer list. Can we retain the layer selection as well?

Not very obvious how to save that setting since Unity serializes the selection list. Also, since we are reworking Layer UI, don't want to spend too much time on this right now.

Sample count and Relative Height under terrain options need tooltips.

Please review comment above, please feel free to write up a tooltip will be happy to add it.

New Visualizers Keep Settings from Previous Ones
Fixed. Bonus - automatically select the newly created visualizer.

Done

@MiroMuse MiroMuse added this to the S18.2.1 milestone Apr 4, 2018
* develop:
  add feature collections and kdtree collection (#565)
  Playmode tests (#628)
  Collider Dropdown in Abstract Map for adding colliders on top of extruded geometry (#637)

# Conflicts:
#	sdkproject/Assets/Mapbox/Examples/1_DataExplorer/InteractiveStyledVectorMap.unity
#	sdkproject/Assets/Mapbox/Unity/Editor/PropertyDrawers/VectorLayerPropertiesDrawer.cs
@abhishektrip abhishektrip merged commit 7a09bc4 into develop Apr 5, 2018
@apavani apavani deleted the ui-fixes branch April 5, 2018 23:22
@abhishektrip abhishektrip mentioned this pull request Apr 17, 2018
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants